fix: keep sibling keywords when a referenced schema has a root-level $ref - #2965
fix: keep sibling keywords when a referenced schema has a root-level $ref#2965RomanHotsiy wants to merge 24 commits into
Conversation
🦋 Changeset detectedLatest commit: c166c21 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Performance Benchmark (Lower is Faster)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…opping resolution
…ocly/openapi-cli into fix/bundle-root-ref-sibling-keywords
Nice performance gain. Do you know the reason? |
|
Verified locally and bisected it — the gain is real and comes from one commit: 094ff48, the sibling-collision fix in 🤖 Addressed by Claude Code |
…alking from resolved children
…ocly/openapi-cli into fix/bundle-root-ref-sibling-keywords
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6fbd859. Configure here.
|
Okay, now it's ready to reveiw. |
vadyvas
left a comment
vadyvas
left a comment
There was a problem hiding this comment.
One stylistic thing, non-blocking: the new code more comments than these files usually are, and a few of the comments are hard to read in themselves
| } = resolve(node); | ||
| const enteredContexts: Set<VisitorLevelContext> = new Set(); | ||
|
|
||
| if (isRefWithSiblings(node)) { |
There was a problem hiding this comment.
found issue with node order in api description:
ref visitors fire twice on a composed $ref, and whether it's once or twice depends on key order.
To reproduce:
// plugin.cjs
module.exports = () => ({
id: 'probe',
rules: {
oas3: {
'count-ref-visits': () => ({
ref(node, ctx) {
if (typeof node.$ref === 'string' && Object.keys(node).length > 1) {
ctx.report({ message: `ref visit: ${ctx.location.pointer}` });
}
},
}),
},
},
});# redocly.yaml
plugins: [./plugin.cjs]
extends: []
rules:
probe/count-ref-visits: error# openapi.yaml
openapi: 3.1.0
info: { title: t, version: 1.0.0 }
paths: {}
components:
schemas:
Usage:
$ref: '#/components/schemas/Composed'
Composed:
$ref: '#/components/schemas/Base'
title: Composed
Base:
type: objectopenapi: 3.1.0
info: { title: t, version: 1.0.0 }
paths: {}
components:
schemas:
# Same three schemas, only reordered: the tree walk reaches Composed first.
Composed:
$ref: '#/components/schemas/Base'
title: Composed
Base:
type: object
Usage:
$ref: '#/components/schemas/Composed'lint reports Composed twice. Move Usage below Base - reports once. On main it's always once
|
|
||
| if (resolvedNode !== undefined && resolvedLocation && type.name !== 'scalar') { | ||
| // the isRef check narrows `node` to OasRef, but sibling keys are read from it too | ||
| const rawNode = node as Record<string, unknown>; |
There was a problem hiding this comment.
do we still need it after adding isRefWithSiblings ?
6 participants
What/Why/How?
bundledropped the sibling keywords of a root-level$refwhen the referenced schema itself started with a$ref— for example an external schema file that composes another file ($ref: ./BaseProblem.yamlplustitle/type/properties/required).The bundled output collapsed such schemas to a bare
$refpointing at the end of the chain, silently losing the sibling constraints.In OpenAPI 3.1, Schema Objects follow JSON Schema draft 2020-12, where
$refwith siblings is a composition, not a transparent alias.Root cause:
followRefinresolve.tschased every chained$refto the end of the chain, discarding the intermediate nodes together with their sibling keys.The fix — resolution results now carry the composition as an additive
chainfield, soctx.resolve()keeps returning the chain-end node exactly as before and nothing changes for existing rules, decorators, or plugins:resolve.ts—followRefstill chases to the end of the chain, but records every composed$refit passes through as achainhop (ResolvedRefChainHop) on the resolve result.walk.ts— the walker walks each chain hop as a ref node from its own location, so hop siblings and their inner$refs are processed;ResolveResultexposes the chain (withLocations) to visitors.bundle-visitor.ts— a chained ref is rewritten to point at the first composed hop (saved as a component with siblings intact) instead of the chain end; the hop's own$refis rewritten by the walker pass. Discriminator mappings and component-name dedup use the same effective target.no-required-schema-properties-undefined.ts— the one rule that needs the composition: it validatesrequiredon chain hops (they are never visited as Schema nodes) and follows chains and siblingproperties/allOf/anyOf/oneOfwhen looking up required properties.rules/utils.ts—resolveSchemapasses the chain through for chain-aware rules.Because
resolve()behavior is unchanged for consumers, the rule regressions found on the earlier revision of this PR (array-parameter-serialization,struct, discriminator checks) cannot occur — verified explicitly for each.The
remove-unused-componentsdecorators are chain-safe by construction (they build their usage graph from raw$refpointers), covered by a new test.Composed schemas are now preserved as components with their sibling keywords intact, references keep pointing at the composed schema, and
--dereferencedmerges the composition in chain order.Reference
Fixes #2964
Testing
--remove-unused-components, a resolver test asserting the recorded chain, and regression tests forstruct,no-required-schema-properties-undefined(sibling-defined, chain-defined, allOf-sibling-defined, and genuinely-missing required properties), andno-unused-components.sibling-refs-root-external-file(fails on unfixed code) andsibling-refs-root-in-file.bundle,bundle --dereferenced,linton the issue repro, and the rule-regression repros from the review.Check yourself
Security
🤖 Generated with Claude Code
Note
Medium Risk
Touches core ref resolution, document walking, and bundling—widely used paths—with behavior changes only when
$refhas sibling keys; coverage is extensive but regressions in edge-case ref chains are possible.Overview
Fixes
bundledropping JSON Schema 2020-12 style compositions where a schema has$refplus sibling keywords (title,properties,required, etc.) when resolution walked through another root-level$ref(e.g. externalBadRequest.yamlthat$refsBaseProblemand adds fields).Resolution still returns the chain-end node, but now attaches a
chainof composed hops (ResolvedRefChainHop) for each$refwith extra keys.walk.tswalks those hops and sibling keys at the ref’s location;bundle-visitor.tssaves components from the first hop (effectiveTarget) so bundled output keeps the composed shape.replaceRefmerges siblings with the resolved object without losing non-object targets.Lint rules that need composition context were updated:
no-required-schema-properties-undefinedfollowschainhops and validatesrequiredon ref nodes;struct/no-unused-componentsregressions are covered. New unit, rule, and e2e fixtures exercise external files, multi-hop chains, discriminator mappings, sibling key collisions, and--remove-unused-components.Reviewed by Cursor Bugbot for commit c166c21. Bugbot is set up for automated code reviews on this repo. Configure here.